Add add_device_terms_to_expression! driver for device-injection methods#112
Closed
luke-kiernan wants to merge 2 commits into
Closed
Add add_device_terms_to_expression! driver for device-injection methods#112luke-kiernan wants to merge 2 commits into
luke-kiernan wants to merge 2 commits into
Conversation
Contributor
|
Performance Results This branch |
Generic driver that applies a per-(device, time) proportional term to one or two balance-expression targets, separating the network-model axis (targets_fn) from the variable/parameter/constant source (term_fn). This lets downstream add_to_expression! methods share a single device/time loop instead of re-implementing it. Stays type-stable / allocation-free on heterogeneous target tuples (e.g. AreaPTDF's String-keyed area expression plus Int-keyed nodal expression) via Base.tail recursion over the statically-sized targets tuple. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
luke-kiernan
force-pushed
the
lk/add-device-terms-driver
branch
from
0ca9dc3 to
42ccd82
Compare
luke-kiernan
changed the base branch from
ac/canonical-key-component-type
to
main
Collaborator
Author
|
I intend to add tests, but I'm waiting until I see if Jose is on board with how I've refactored things in the companion POM PR. I'm using closures, which we usually try to avoid. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new exported helper API to factor out the common device × time looping pattern used by downstream “device injection” add_to_expression! implementations, centralizing the looping logic in IOM while letting downstream code provide target selection and per-device term generation.
Changes:
- Export
add_device_terms_to_expression!from the main module. - Add
add_device_terms_to_expression!generic driver to apply per-device/per-time proportional terms to 1–2 expression targets. - Add internal
_apply_term_to_targets!helper using tuple tail recursion to keep heterogeneous target tuples type-stable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/InfrastructureOptimizationModels.jl |
Exports the new public driver function. |
src/common_models/add_jump_expressions.jl |
Implements the new driver and its internal target-application helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+100
to
+101
| # nodal entry plus a system/area entry). Tail recursion ensures type stability for | ||
| # the hetereogeneous length 2 tuples. |
Comment on lines
+102
to
+110
| const _BalanceTermValue = Union{Float64, JuMP.AbstractJuMPScalar} | ||
|
|
||
| _apply_term_to_targets!(::Tuple{}, ::_BalanceTermValue, ::Float64, ::Int) = nothing | ||
|
|
||
| function _apply_term_to_targets!( | ||
| targets::Tuple, | ||
| value::_BalanceTermValue, | ||
| multiplier::Float64, | ||
| t::Int, |
Member
|
Lets merge this once POM is cleared up from the pending work |
jd-lara
reviewed
Jun 20, 2026
| The same `value * multiplier` term is added to every target via | ||
| [`add_proportional_to_jump_expression!`](@ref). | ||
| """ | ||
| function add_device_terms_to_expression!( |
Member
There was a problem hiding this comment.
I am not convinced passing a function here is a clean option. This function can't never compile and cause invalidations. This implementation is very pythonic
Collaborator
Author
There was a problem hiding this comment.
Yeah, there's definitely some code smell. I agree that this solution feels clunky. I didn't check the compilation consequences of having a function stub in a hotloop.
I can remove the function closures and eliminate add_device_terms_to_expression! here in IOM. However, I'd like to keep the target resolution code in the POM PR. Concretely: in the variable case, we'd have
function add_to_expression!(container, ::Type{T}, ::Type{U}, devices, ::DeviceModel{V,W}, network_model) where {...}
variable = get_variable(container, U, V)
mult = get_variable_multiplier(U, V, W)
time_steps = get_time_steps(container)
for d in devices
targets = _balance_expression_targets(container, T, network_model, d)
name = PSY.get_name(d)
for t in time_steps
_apply_term_to_targets!(targets, variable[name, t], mult, t)
end
end
return
endwhereas right now on main, the network model specifics inside _balance_expression_targets are copy-pasted (ref-bus lookup for CopperPlate, area lookup for AreaBalance, etc.) and _apply_term_to_targets! is 1 (non-PTDF) or 2 (PTDF) calls to add_proportional_to_jump_expression!
Collaborator
Author
There was a problem hiding this comment.
I didn't check the compilation consequences of having a function stub in a hotloop.
Wait. I saw _apply_term_to_targets!(::Tuple{}, ::_BalanceTermValue, ::Float64, ::Int) = nothing and thought I had a stub function, but no, this is tail recursion. I'm convinced there is no actual compilation or type stability issue here.
Full reasoning
For a single call site (one concrete `T` , `network_model`, term source):targets_fn::F, term_fn::G— the where{F<:Function, G<:Function}forces Julia to specialize the driver per closure type.F/Gare concrete inside any given specialization, not abstractFunction. No dynamic dispatch to call them.targets = targets_fn(d)— dispatches to exactly one_balance_expression_targetsmethod (Tandnetwork_modelare concrete), so the tuple structure (1- vs 2-element, and each element's index type) is fixed and concrete._apply_term_to_targets!recursion — operates on a concrete 1-or-2 tuple;Base.tailshortens it by one each step, terminating at the::Tuple{}method. This fully unrolls at compile time and handles the heterogeneous 2-tuple (system-keyed + bus-keyed) correctly.scale::Sas a positional type parameter —isnothing(scale)folds to a compile-time constant, so the dead branch is pruned. No instability, noscale(d)dynamic call.- Closure captures — I had Claude check each closure:
name,multiplier,variable,refsare each assigned exactly once before the innert -> …is built. No reassignment of a captured variable → noCore.Box, no boxing instability.
Two closures branch and return different inner closures, t -> ... lambdas with distinct types. That's a little messy: term = term_fn(d) infers as Union{Closure1, Closure2}. Thanks to union-splitting, though, the performance cost should be next to zero.
I can go run some profiling to confirm. If the code smell is too much, I'm on board with letting this go and sticking with what I suggested above...but I'm pretty confident that compilation/invalidations here aren't an issue.
Collaborator
Author
|
Closing: superseded by review feedback. The higher-order |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
add_device_terms_to_expression!— a generic driver for device-injectionadd_to_expression!methods — plus its internal_apply_term_to_targets!helper, and exports the driver.Why
Downstream (POM) has a large family of
add_to_expression!methods that are the samedevice × timeloop differing only in (a) which expression entries a device contributes to (network-model dependent) and (b) the variable/parameter/constant term. This driver factors out the loop, so those methods collapse to a small target resolver + a term closure. The POM-side consolidation lands separately (POM #135).Design
targets_fn(d)returns a 1- or 2-element tuple of(expression_matrix, row_index)targets (one for single-bus/area/system models; two for PTDF/AreaPTDF).term_fn(d)returns a per-devicet -> (value, multiplier)closure._apply_term_to_targets!consumes the targets viaBase.tailrecursion rather than aforloop: this stays type-stable and allocation-free even when the tuple is heterogeneous (AreaPTDF mixes aString-keyed area expression and anInt-keyed nodal expression, which aforloop would box). Verified identical native codegen and 0 allocations.Target branch
Into
main. (Additive — the two touched files are identical betweenmainandac/canonical-key-component-type, so it applies cleanly to either.)🤖 Generated with Claude Code